Skip to content

C#: Loop unrolling for foreach statements#1549

Merged
calumgrant merged 8 commits into
github:masterfrom
hvitved:csharp/cfg/loop-unrolling
Sep 12, 2019
Merged

C#: Loop unrolling for foreach statements#1549
calumgrant merged 8 commits into
github:masterfrom
hvitved:csharp/cfg/loop-unrolling

Conversation

@hvitved

@hvitved hvitved commented Jul 4, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR adds loop unrolling for foreach statements. For example, the CFG for

void M1(string[] args)
{
    if (args.Length == 0)
        return;
    foreach(var arg in args) // unroll
        Console.WriteLine(arg);
}

is now
Screenshot 2019-09-03 at 09 39 41

The QL code for loop unrolling is made general enough that we can later add loop unrolling for other types of loop constructs (for example for loops).

The dist-compare report is here (internal link), and I recommend commit-by-commit review.

@hvitved hvitved force-pushed the csharp/cfg/loop-unrolling branch from 812f77d to aa069fe Compare July 5, 2019 12:41
@hvitved hvitved force-pushed the csharp/cfg/loop-unrolling branch 5 times, most recently from 5759ffd to ab09056 Compare August 30, 2019 11:42
@hvitved hvitved force-pushed the csharp/cfg/loop-unrolling branch 2 times, most recently from 1047c0a to 706090d Compare August 30, 2019 13:25
@hvitved hvitved force-pushed the csharp/cfg/loop-unrolling branch from 706090d to 6752557 Compare September 1, 2019 08:34
@hvitved hvitved added the C# label Sep 3, 2019
@hvitved hvitved marked this pull request as ready for review September 3, 2019 07:43
@hvitved hvitved requested a review from a team as a code owner September 3, 2019 07:43
@hvitved hvitved requested a review from calumgrant September 3, 2019 07:43
@calumgrant

calumgrant commented Sep 6, 2019

Copy link
Copy Markdown
Contributor

Could this strategy for unrolling also make sense? image

calumgrant
calumgrant suggested changes Sep 6, 2019
View reviewed changes

@calumgrant calumgrant left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really love this idea. It will definitely help make some queries more precise and reduce false positives.

Comment thread csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll
abstract predicate isSingleton();

/**
* Holds if this value describes a referential property. For example, emptiness

@calumgrant calumgrant Sep 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm struggling with the term "referential property". I understand the concept but the name makes no sense to me.

@calumgrant calumgrant Sep 6, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be a "transient property" or "readonly property" instead?

@hvitved hvitved Sep 9, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is that it is a property of the thing that an expression references, not the value of the expression itself. Is "reference property" better?

@calumgrant calumgrant Sep 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that just "property"? How about abstract property? I don't really know.

Comment thread csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll Outdated
Comment thread csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll Outdated
Comment thread csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll
Comment thread csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll
Comment thread csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll Outdated
Comment thread csharp/ql/src/semmle/code/csharp/frameworks/system/collections/Generic.qll
Comment thread csharp/ql/src/semmle/code/csharp/controlflow/internal/Splitting.qll
Comment thread csharp/ql/src/semmle/code/csharp/dataflow/internal/BaseSSA.qll
def1.getEnclosingCallable() != def2.getEnclosingCallable()
)
}
SimpleLocalScopeVariable() { not getAnAssigningCallable(this) != getAnAssigningCallable(this) }

@calumgrant calumgrant Sep 6, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't change this here, but words like "Simple" don't convey enough information. Perhaps "UncapturedLocalScopeVariable" etc.

Comment thread change-notes/1.23/analysis-csharp.md
@hvitved

hvitved commented Sep 9, 2019

Copy link
Copy Markdown
Contributor Author

Could this strategy for unrolling also make sense?

Certainly, that should just make things simpler (less splitting). I guess I got so focused on the "loop unrolling" terminology, that I forgot that all we need to unroll is the loop condition.

@hvitved

hvitved commented Sep 10, 2019

Copy link
Copy Markdown
Contributor Author

Updated dist-compare report: https://git.semmle.com/gist/tom/e8904f2fc636c0ddeebe4d777203e8f4#file-report2-md.

calumgrant
calumgrant reviewed Sep 11, 2019
View reviewed changes

@calumgrant calumgrant left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Optionally, use implies if you think it's clearer.

Comment thread csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll
Comment thread csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll
calumgrant
calumgrant approved these changes Sep 12, 2019
View reviewed changes

@calumgrant calumgrant left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!

@calumgrant calumgrant merged commit e330d5a into github:master Sep 12, 2019
@hvitved hvitved deleted the csharp/cfg/loop-unrolling branch September 12, 2019 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@calumgrant calumgrant calumgrant approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

C#

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hvitved @calumgrant